Repository navigation
fix(desktop): keep connection detail sibling keys distinct - #6272
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed both changed files at b74adde0e76ed80a01f165e82fe7f631b3d30559. I found no confirmed P0–P2 issue.
ConnectionDefaultAction now uses a prefixed connection-specific key, distinct from its ConnectionDetail sibling (apps/desktop/src/renderer/settings/providers-panel.tsx:361). Both keys still change with the selected connection, preserving the intended per-connection remount. The added mounted-render regression checks the actual connection-detail surface for duplicate-key errors (connection-settings-locale-render.test.ts:562).
Fresh dependencies/patches, the normal root build, renderer build and renderer typecheck passed. All 41 connection-settings render tests passed. Supplying only main's actual previous providers-panel.tsx to the same test's renderer bundler makes the new regression fail with React's duplicate relay-connection key warning. Changed-file lint, strict renderer architecture, ASF headers, diff checks, epoch guard (228; no protocol change) and static merge passed. Current-head test/label CI is successful and the PR is mergeable.
P3: the PR's AI-use choices are both still unchecked. Please select the applicable statement and, if generative tooling made a substantive contribution, identify it and preserve the required commit attribution.
I did not run a packaged Electron/CDP session or the complete Desktop/repository suites. The mounted tests use the real renderer components with controlled bridge responses.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Reviewed at exact head b74adde0e76ed80a01f165e82fe7f631b3d30559.
Finding
- [P2] Complete the required AI-use declaration before merge. The PR body leaves both AI-use choices unchecked and the “Tool(s) and scope” field empty.
CONTRIBUTING.mdrequires every pull request to state whether generative tooling contributed substantively. Please select exactly one option. If generative tooling did contribute materially, name the tool and scope and add aGenerated-by: <tool>trailer to every affected commit; otherwise select the no-substantive-contribution option.
The implementation itself is clean. The prefixed key remains connection-specific, so ConnectionDefaultAction still remounts when the selected connection changes while no longer colliding with its ConnectionDetail sibling. On the current-main synthetic merge (574f52bd0e7ca4a19702ebc90b4f3eedb6068a20, tree 6dde90a2febea0b7402d083a2b9b26c5fa968be5), the full workspace test build, Desktop typecheck, strict renderer architecture check, all 41 affected render tests, Biome, and the diff check passed. Reverting only the key prefix made the new production-component regression fail with React's duplicate relay-connection key warning; restoring it made the test pass. Exact-head hosted checks are successful.
Conclusion: BLOCKED only on the required PR declaration; no implementation P0–P2 findings. I did not run a packaged Electron/CDP session or the complete Desktop/repository test suites.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Opening any connection's detail logged "Encountered two children with the same key": ConnectionDefaultAction (keyed per connection in apache#5866 so its state resets when the selection changes) and ConnectionDetail both used the connection id as their key under the same level. Prefix the default action's key so both still remount per connection without colliding, and cover it in the providers-panel render test. Generated-by: Claude Code
b74adde to
f44ee37
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
APPROVE at exact head f44ee37ed6697303f4f7623e598e8105c2258c7a. There are no unresolved P0–P2 findings.
The previous AI-attribution finding is closed: the pull request now declares the substantive use of Claude Code / Claude Opus 5.5 and its scope, and the commit contains Generated-by: Claude Code. The amended commit has the same source tree (fab86e9905843762f17d832fbc45ef06abe39e87) as the fully reviewed head, so the technical evidence remains applicable.
The connection-specific prefix keeps ConnectionDefaultAction remounting when the selected connection changes while preventing its key from colliding with the sibling ConnectionDetail. The clean merge with current main 574f52bd0e7ca4a19702ebc90b4f3eedb6068a20 produced tree 6dde90a2febea0b7402d083a2b9b26c5fa968be5. The workspace test build, Desktop renderer typecheck, strict renderer architecture check, all 41 affected render tests, Biome, and the diff check passed. Restoring only the old key made the new regression fail with React's duplicate relay-connection key warning; restoring the prefix made it pass. The hosted test check for this exact head completed successfully.
I did not run a packaged Electron/CDP session or the complete Desktop/repository test suites. This approval does not merge the pull request.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Summary
Opening any connection's detail page (Settings → Models → a connection) logs React's "Encountered two children with the same key,
<connectionId>". #5866 keyedConnectionDefaultActionby the connection id so its dialog state resets when the selection changes, but its siblingConnectionDetailunder the same level is keyed by the same id. This prefixes the default action's key, so both still remount per connection and the keys no longer collide.Verification
connection-settings-locale-render.test.ts: fails on main (7 duplicate-key errors forrelay-connection), passes with the fix; the file's 41 tests pass.tsc -p tsconfig.renderer.json/tsconfig.main.jsonclean;check-renderer-architecture.mjspasses, also with--base upstream/main --strict-base;biome linton both files clean.MAKA_E2E_FIXTURE=settings-connections,dev:hmr), driven over CDP: opening both fixture connections' details logged 2 duplicate-key errors with main's key and 0 console errors/warnings with the fix.AI use
Select exactly one:
Tool(s) and scope: Claude Code (Claude Opus 5.5) diagnosed the duplicate key, wrote the key fix, the regression test and this description, and ran the verification listed above. The commit carries
Generated-by: Claude Code.Checklist
Does this PR entail a change in behavior?